[v10.x] tls: group chunks into TLS segments#28904
Closed
mildsunrise wants to merge 3 commits intonodejs:v10.x-stagingfrom
Closed
[v10.x] tls: group chunks into TLS segments#28904mildsunrise wants to merge 3 commits intonodejs:v10.x-stagingfrom
mildsunrise wants to merge 3 commits intonodejs:v10.x-stagingfrom
Conversation
Correct docs to clarify that behaviour, and fix a race condition in test-http2-large-write-destroy.js. Fixes: nodejs#27863 PR-URL: nodejs#27891 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#28903 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
TLSWrap::DoWrite() now concatenates data chunks and makes a single call to SSL_write(). Grouping data into a single segment: - reduces network overhead: by factors of even 2 or 3 in usages like `http2` or `form-data` - improves security: segment lengths can reveal lots of info, i.e. with `form-data`, how many fields are sent and the approximate length of every individual field and its headers - reduces encryption overhead: a quick benchmark showed a ~30% CPU time decrease for an extreme case, see nodejs#27573 (comment) Fixes: nodejs#27573 PR-URL: nodejs#27861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot
added
c++
Collaborator
Collaborator
Member
|
Tests are passing. /ping @nodejs/backporters for reviews/landing |
Member
|
@nodejs/crypto |
bnoordhuis
approved these changes
Aug 3, 2019
Member
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
Thanks, LGTM.
Instead I've defined it as
vector<char>which has almost identical API; are these changes okay for backports?
Ideally the delta between release lines is as small as possible. If it's not a gargantuan task to back-port the AllocatedBuffer API, then that's strongly preferred.
Contributor
Author
|
Then I'll try to also backport #26207 and see if it works. |
Contributor
Author
Member
|
Thanks, at least you tried. I'm good with landing this PR as is. |
BethGriggs
approved these changes
Oct 7, 2019
BethGriggs
pushed a commit
that referenced
this pull request
Oct 7, 2019
Correct docs to clarify that behaviour, and fix a race condition in test-http2-large-write-destroy.js. Fixes: #27863 Backport-PR-URL: #28904 PR-URL: #27891 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs
pushed a commit
that referenced
this pull request
Oct 7, 2019
TLSWrap::DoWrite() now concatenates data chunks and makes a single call to SSL_write(). Grouping data into a single segment: - reduces network overhead: by factors of even 2 or 3 in usages like `http2` or `form-data` - improves security: segment lengths can reveal lots of info, i.e. with `form-data`, how many fields are sent and the approximate length of every individual field and its headers - reduces encryption overhead: a quick benchmark showed a ~30% CPU time decrease for an extreme case, see #27573 (comment) Fixes: #27573 Backport-PR-URL: #28904 PR-URL: #27861 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Member
|
Landed in 7f48519...fe58bca |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #27861 (and also #27891 and #28903 to fix tests).
The original code uses
AllocatedBuffer, so we would need to also backport #26207 (specifically 6c257cd). Instead I've defined it asvector<char>which has almost identical API; are these changes okay for backports?